Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finance USD conversion mechanism − no string manipulation #1180

Conversation

bpierre
Copy link
Contributor

@bpierre bpierre commented Jun 25, 2020

No description provided.

: new BN('-1'),
amountConverted:
amount && decimals && convertRates[symbol]
? convertAmount(amount, decimals, 1 / convertRates[symbol])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we can also get a convertRate that is > 1 here (potentially, many times greater).

return formatTokenAmount(
amount
.mul(new BN(10).pow(new BN(CONVERT_PRECISION)))
.mul(new BN(rate * 10 ** CONVERT_PRECISION)),
Copy link
Contributor

@sohkai sohkai Jun 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not entirely sure on this; you're going to get an error if rate * 10 ** CONVERT_PRECISION becomes too large or is still an imprecise number.

@coveralls
Copy link

coveralls commented Jun 25, 2020

Coverage Status

Coverage remained the same at 97.642% when pulling af337d2 on fix-finance-usd-conversion-no-string-manipulation into 932c6e4 on fix-finance-usd-conversion.

@bpierre
Copy link
Contributor Author

bpierre commented Jun 25, 2020

@sohkai You are right, it wouldn’t work since the rate can be larger than this. Closing this PR, I moved the Jest config back to #1177.

@bpierre bpierre closed this Jun 25, 2020
@sohkai sohkai deleted the fix-finance-usd-conversion-no-string-manipulation branch July 17, 2020 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants